Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 554 clean #598

Merged
merged 36 commits into from
Nov 19, 2021
Merged

Issue 554 clean #598

merged 36 commits into from
Nov 19, 2021

Conversation

pierrejego
Copy link
Member

Lots of modification in this PR

-> update all libs
-> update geotools management to generate image
-> change CXF to springMVC
-> replace headers information management by input in MDC ( for maintenance purpose )
-> Compatibility with java11
-> add swagger

pierrejego and others added 30 commits September 14, 2021 17:35
Change CXF to Spring MVC, that's why most of the java class and configuration had to be updated
Use MDC possibilities for CNIL level and filtering authorisation (roles and org)
Geotools update made the code slightly change for image generation

All in one commit because there were dependencies beetween libraries
add missing space in logback configuration
… at the sametime. Add /services/* mapping in web.xml and not in RequestMapping
…ple specific versionning

Remove unused import
Correction on datastoire connection, replacement : in _ not needed anymore
Add debug logs to see all available layers name ( to help to see if geo_parcelle not correctly deployed)
Change File to byte[], no need to write on disk anymore (save some times)
… "attachement" content-disposition in headers
Add request execution time
Corrections BDD et Demandes

Build Docker failed - pb de login docker hub
@pierrejego pierrejego self-assigned this Nov 19, 2021
@pierrejego pierrejego added this to the v 1.10 milestone Nov 19, 2021
@jusabatier
Copy link
Collaborator

J'ai juste une question concernant MDC :
Je ne connaissait pas cette fonction, et j'ai constaté qu'elle fait partie de slf4j est est destinée à du logging.

Est-ce suffisamment fiable pour gérer la sécu d'une application ?

De ce que j'ai pu lire ici : https://www.baeldung.com/mdc-in-log4j-2-logback (Partie 6)
Une mauvaise gestion des thread peut provoquer des erreurs.

@pierrejego
Copy link
Member Author

Oui aucun soucis de ce côté, ,il faut bien s'assurer d'avoir les put et remove c'est pour ça que je l'ai mis dans les interceptors.
Et si jamais le thread courant lançait un autre thread il faudrait s'assurer de faire la même mécanique, mais nous n'avons pas le cas dans cadastrapp

@jusabatier
Copy link
Collaborator

Si le thread courant crash (Exception), les remove ne seront pas appliqués.

Il faudrait faire un MDC.clear(); dans la methode afterCompletion de CadastrappInterceptor au lieu de nettoyer dans le postHandle (uniquement appelé en cas de succès)

Sources : https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/servlet/HandlerInterceptor.html

@pierrejego
Copy link
Member Author

pierrejego commented Nov 19, 2021

Il faudrait faire un MDC.clear(); dans la methode afterCompletion de CadastrappInterceptor au lieu de nettoyer dans le postHandle (uniquement appelé en cas de succès)

Oui bonne idée, je le rajoute, c'est plus propre
mais vu qu'on fait un put à chaque entrée les données sont écrasé de toute façon

@MaelREBOUX
Copy link
Member

Je suis totalement dépassé !
on va dire que c'est bon signe 😅

@landryb
Copy link
Member

landryb commented Nov 19, 2021

la PR est monstrueuse (réindentation etc) donc difficile a review dans l'état, mais ca build et ca s'installe OK en dev ici.

@jusabatier
Copy link
Collaborator

Concernant les logs, en utilisant INFO j'ai ça :

2021-11-19 16:44:26,318 [http-nio-8680-exec-2] INFO //cadastrapp/services/getParcelle - julien.sabatier - ROLE_SUPERUSER;ROLE_OPENADS_SOAP;ROLE_MAPSTORE_ADMIN;ROLE_TERRASSES_MANAGER;ROLE_CADASTRAPP_CNIL2;ROLE_USER;ROLE_ADMINISTRATOR;ROLE_OPENADS;ROLE_EXTRACTORAPP;ROLE_GN_ADMIN;ROLE_POSTGRESQL_ADMINISTRATOR -CAPEV -o.g.c.p.CadastrappInterceptor - Incoming request
2021-11-19 16:44:26,397 [http-nio-8680-exec-2] INFO //cadastrapp/services/getParcelle - julien.sabatier - ROLE_SUPERUSER;ROLE_OPENADS_SOAP;ROLE_MAPSTORE_ADMIN;ROLE_TERRASSES_MANAGER;ROLE_CADASTRAPP_CNIL2;ROLE_USER;ROLE_ADMINISTRATOR;ROLE_OPENADS;ROLE_EXTRACTORAPP;ROLE_GN_ADMIN;ROLE_POSTGRESQL_ADMINISTRATOR -CAPEV -o.g.c.p.CadastrappInterceptor - Send response

On a 2 ligne par requête, pas sur que ce soit nécessaire.

@pierrejego
Copy link
Member Author

pierrejego commented Nov 19, 2021

Concernant les logs, en utilisant INFO j'ai ça :

2021-11-19 16:44:26,318 [http-nio-8680-exec-2] INFO //cadastrapp/services/getParcelle - julien.sabatier - ROLE_SUPERUSER;ROLE_OPENADS_SOAP;ROLE_MAPSTORE_ADMIN;ROLE_TERRASSES_MANAGER;ROLE_CADASTRAPP_CNIL2;ROLE_USER;ROLE_ADMINISTRATOR;ROLE_OPENADS;ROLE_EXTRACTORAPP;ROLE_GN_ADMIN;ROLE_POSTGRESQL_ADMINISTRATOR -CAPEV -o.g.c.p.CadastrappInterceptor - Incoming request
2021-11-19 16:44:26,397 [http-nio-8680-exec-2] INFO //cadastrapp/services/getParcelle - julien.sabatier - ROLE_SUPERUSER;ROLE_OPENADS_SOAP;ROLE_MAPSTORE_ADMIN;ROLE_TERRASSES_MANAGER;ROLE_CADASTRAPP_CNIL2;ROLE_USER;ROLE_ADMINISTRATOR;ROLE_OPENADS;ROLE_EXTRACTORAPP;ROLE_GN_ADMIN;ROLE_POSTGRESQL_ADMINISTRATOR -CAPEV -o.g.c.p.CadastrappInterceptor - Send response

On a 2 ligne par requête, pas sur que ce soit nécessaire.

Je l'ai mis pour pouvoir faire des stats sur les logs avec Kibana après. (Entrée-sortie et du coup indirectement temps de traitement) Mais ça n'a pas une grand plus value effectivement. @MaelREBOUX @landryb vous en pensez quoi je passe la trace en debug ?

@jusabatier
Copy link
Collaborator

En revanche pour de la stat, je pense que ça serait très utile d'avoir des logs des différentes extractions ou génération qui sont faites du logiciel.

Par exemple :

  • Exports CSV
  • Génération BP/RP

Qui incluraient :

  • Horodatage
  • Utilisateur
  • Paramêtres requete pertinents (contient les infos prop / parcelle interogée / compte communal interogé )

@pierrejego
Copy link
Member Author

En revanche pour de la stat, je pense que ça serait très utile d'avoir des logs des différentes extractions ou génération qui sont faites du logiciel.

Par exemple :

  • Exports CSV
  • Génération BP/RP

Qui incluraient :

  • Horodatage
  • Utilisateur
  • Paramêtres requete pertinents (contient les infos prop / parcelle interogée / compte communal interogé )

Je l'ai fait en mode log debug pour pas impacté les perfs
Ici pour l'entrant avec les paramètres
https://github.com/georchestra/cadastrapp/blob/issue-554-clean/cadastrapp/src/main/java/org/georchestra/cadastrapp/providers/CadastrappInterceptor.java#L48

ici en sortant pour le temps de traitement
https://github.com/georchestra/cadastrapp/blob/issue-554-clean/cadastrapp/src/main/java/org/georchestra/cadastrapp/providers/CadastrappInterceptor.java#L68

@jusabatier
Copy link
Collaborator

Ok je regarderais ça plus en détails.

Pour l'instant je fait mes stats d'utilisations cadastrapp depuis les logs du secproxy et ça marche pas trop mal.
On verra si il y a besoin d’améliorer des choses dans une autre issue.

Pour le reste de ce que j'ai testé ça fonctionne donc OK pour moi pour pousser sur master et taguer comme nouvelle version.

@pierrejego
Copy link
Member Author

la PR est monstrueuse (réindentation etc) donc difficile a review dans l'état, mais ca build et ca s'installe OK en dev ici.

Je prends ça pour un approved :)

@pierrejego pierrejego merged commit 10584ac into master Nov 19, 2021
@landryb
Copy link
Member

landryb commented Nov 22, 2021

la PR est monstrueuse (réindentation etc) donc difficile a review dans l'état, mais ca build et ca s'installe OK en dev ici.

Je prends ça pour un approved :)

oui c'etait un approved :) quand je disais monstrueuse, c'est aussi qu'en général c'est bien mieux de faire une PR par fonctionalité avec peu de commits (et donc X PRs), plutot que plein de trucs différents et pas forcément liés les uns aux autres dans la meme PR ;)

@MaelREBOUX
Copy link
Member

@MaelREBOUX @landryb vous en pensez quoi je passe la trace en debug ?

ben moi vu que j'ai accès à rien : ça me fait une belle jambe. Mais sinon oui je pencherai plutôt pour mettre ça en debug pour pas alourdir des log.
Mais c'est intéressant de savoir ça.

Il faudrait donc documenter ça ici : https://docs.georchestra.org/cadastrapp/latest/guide_administrateur/configuration_webapp.html

Je te fais une issue pour ça ?

En revanche pour de la stat, je pense que ça serait très utile d'avoir des logs des différentes extractions ou génération qui sont faites du logiciel.

Par exemple :

  • Exports CSV
  • Génération BP/RP

Qui incluraient :

  • Horodatage
  • Utilisateur
  • Paramêtres requete pertinents (contient les infos prop / parcelle interogée / compte communal interogé )

Je suis prêt à coucher pour avoir ça dans une table dans la BD...

@jusabatier
Copy link
Collaborator

Pas forcement très compliqué à mettre en place, il faut juste définir un logger avec ces infos et le brancher sur une BDD.
Je l'ai déjà fait sur mon sec-proxy pour analyser l'utilisations des différentes apps.

Oui tu peux faire une issue pour garder une trace du besoin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants